Skip to content

Use patch instead of update to replace sidecars with nop image #9128

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Use patch instead of update to replace sidecars with nop image #9128

wants to merge 5 commits into from
+59 −28

Conversation

@prad9192
Copy link

@prad9192 prad9192 commented Nov 7, 2025 •
edited by vdemeester
Loading

What

Changed StopSidecars() to use Patch() instead of Update() when stopping sidecar containers by replacing their images with the nop image.

Why

The original implementation used Update(), which requires an exact resourceVersion match. This causes 409 conflicts when the kubelet updates the pod status between our GET and UPDATE calls (which happens frequently as containers terminate).

This causes the below errors on the pod.

error stopping sidecars of Pod: Operation cannot be fulfilled on pods "...": 
the object has been modified; please apply your changes to the latest version

And the task runs fail with TaskRunResolutionFailed

image

Even though the task succeeded and sidecars eventually stop, the TaskRun gets marked as failed due to this race condition.
The fix uses JSON Patch (same pattern as UpdateReady() and CancelPod() in this file), which only patches the specific container image fields

Release Notes

Does this PR introduce a user-facing change?

Fixed race condition causing TaskRuns to fail with 409 conflict error when stopping sidecars. 
StopSidecars now uses Patch instead of Update to avoid conflicts with concurrent kubelet pod status updates.

@tekton-robot tekton-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Nov 7, 2025
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 7, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: prad9192 / name: Pradeep Ashwathanarayan (d68af40)

@tekton-robot
Copy link
Collaborator

tekton-robot commented Nov 7, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vdemeester after the PR has been reviewed.
You can assign the PR to them by writing /assign @vdemeester in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 7, 2025
@prad9192 prad9192 force-pushed the tekton-stop-sidecar-patch-instead-of-update branch from 934d222 to 434ad9a Compare November 7, 2025 01:22
@prad9192
Copy link
Author

prad9192 commented Nov 9, 2025

/retest

@waveywaves waveywaves added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Nov 13, 2025
@tekton-robot tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 18, 2025
@vdemeester
Copy link
Member

vdemeester commented Nov 18, 2025

@prad9192 you will need to rebase against main branch. The e2e failure are fixed in the latest main commits.

@prad9192
Copy link
Author

prad9192 commented Nov 18, 2025

/label kind/bug

@tekton-robot
Copy link
Collaborator

tekton-robot commented Nov 18, 2025

@prad9192: The label(s) /label kind/bug cannot be applied. These labels are supported: ``

In response to this:

/label kind/bug

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@prad9192
Copy link
Author

prad9192 commented Nov 18, 2025

/retest

@AlanGreene
Copy link
Member

AlanGreene commented Nov 18, 2025

/kind bug

@tekton-robot tekton-robot added the kind/bug Categorizes issue or PR as related to a bug. label Nov 18, 2025
afrittoli
afrittoli reviewed Nov 19, 2025
View reviewed changes
Copy link
Member

@afrittoli afrittoli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this fix, it's much appreciated!
Could you add some testing for the new functionality? It should be possible to simulate the situation you described in the PR description of the pod being updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afrittoli afrittoli afrittoli left review comments

@jerop jerop Awaiting requested review from jerop

@twoGiants twoGiants Awaiting requested review from twoGiants

Assignees

No one assigned

Labels

kind/bug Categorizes issue or PR as related to a bug. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

Tekton Community Roadmap
Status: Todo

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@prad9192 @tekton-robot @vdemeester @AlanGreene @afrittoli @waveywaves